Rework migration and on runtime upgrade logic execution by dmitrylavrenov · Pull Request #1315 · humanode-network/humanode

dmitrylavrenov · 2024-11-01T11:32:51Z

The main goal of this PR is to avoid running runtime upgrade execution for each pallet on each runtime upgrade. The idea is taken from substrate and polkadot-sdk approach to use an ability passing actual migration at the runtime itself.

There are introduced separate objects responsible for runtime upgrade logic that implement OnRuntimeUpgrade to be passed at humanode-runtime to Executive like:

/// Executive: handles dispatch to the various modules.
pub type Executive = frame_executive::Executive<
    Runtime,
    Block,
     frame_system::ChainContext<Runtime>,
     Runtime,
     AllPalletsWithSystem,
     (
         init_storage_version::InitStorageVersion<DummyPrecompilesCode, Runtime>,
         init_storage_version::InitStorageVersion<BalancedCurrencySwapBridgesInitializer, Runtime>,
     ),
 >;

crates/humanode-runtime/src/init_storage_version.rs

crates/pallet-balanced-currency-swap-bridges-initializer/src/tests.rs

MOZGIII

Please explain the logic behind this change - in particular why alter all the pallets

dmitrylavrenov · 2024-11-08T13:12:57Z

Please explain the logic behind this change - in particular why alter all the pallets

Added to the description

MOZGIII · 2024-11-21T13:05:42Z

We'd have to put this on hold for now, a I need to do more research on this and dig deeper to really conclude something propely; note there are no useless migrations, and, in principle, migrations are to be kept forever; unless there is a special certain cutoff date for them.

dmitrylavrenov · 2024-11-21T13:13:11Z

We'd have to put this on hold for now, a I need to do more research on this and dig deeper to really conclude something propely; note there are no useless migrations, and, in principle, migrations are to be kept forever; unless there is a special certain cutoff date for them.

Got it. The PR is not about avoiding having useless migrations, but to make the migrations itself more flexible. We can always have it at runtime level to keep it forever but control all migrations in one place.

Btw, looking forward to doing more research on your side and discussing later. I just was inspired by the general logic of custom upgrades usage after researching substrate updates related to polkadot-v0.9.43.

…dges-initializer

MOZGIII · 2024-11-29T15:19:54Z

crates/pallet-dummy-precompiles-code/src/tests.rs


-            // Do runtime upgrade hook.
-            v1::AllPalletsWithoutSystem::on_runtime_upgrade();
+            // // Do runtime upgrade hook.


Suggested change

// // Do runtime upgrade hook.

// Do runtime upgrade hook.

crates/pallet-dummy-precompiles-code/src/mock/v1.rs

Copilot

PR Overview

This PR reworks the migration and runtime upgrade logic to avoid invoking upgrades for each pallet on every runtime upgrade. The key changes include dedicated OnRuntimeUpgrade implementations for migration (e.g. UpgradeInit, MigrationV0ToV1) and removal of redundant runtime hooks, along with updates to Cargo.toml dependency lists to support the new design.

Reviewed Changes

File	Description
crates/pallet-dummy-precompiles-code/src/upgrade_init.rs	Introduces a dedicated runtime upgrade implementation for dummy precompiles
crates/pallet-erc20-support/src/migrations/v1.rs	Adds migration logic from version 0 to 1 with minor spelling corrections
crates/pallet-humanode-session/src/migrations/v1.rs	Updates migration logic and error messages with corrected version control text
crates/pallet-balanced-currency-swap-bridges-initializer/Cargo.toml	Updates Cargo.toml dependencies to include frame-executive features
crates/pallet-dummy-precompiles-code/Cargo.toml	Updates Cargo.toml dependencies to include frame-executive features
Other mock and lib files	Adjust runtime upgrade integration to use the new UpgradeInit implementations

Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.

Copilot · 2025-03-04T21:47:28Z

crates/pallet-dummy-precompiles-code/src/upgrade_init.rs

+        }
+
+        if !not_created_precompiles.is_empty() {
+            return Err("precompiles not created properly: {:not_created_precompiles}");


The error message uses a placeholder without interpolation. Consider using a formatted string (e.g. via format!) to include the relevant data.

Suggested change

return Err("precompiles not created properly: {:not_created_precompiles}");

return Err(format!("precompiles not created properly: {:?}", not_created_precompiles).as_str());

Copilot · 2025-03-04T21:47:29Z

crates/pallet-erc20-support/src/migrations/v1.rs

+    let mut weight: Weight = T::DbWeight::get().reads(1);
+
+    if onchain_version != 0 {
+        info!("Not at version 0, nothing to do. This migrarion probably should be removed");


Spelling error in 'migrarion'; please change it to 'migration'.

Suggested change

info!("Not at version 0, nothing to do. This migrarion probably should be removed");

info!("Not at version 0, nothing to do. This migration probably should be removed");

Copilot · 2025-03-04T21:47:29Z

crates/pallet-humanode-session/src/migrations/v1.rs

-    if onchain == 1 {
-        info!("Already at version 1, nothing to do");
+    if onchain != 0 {
+        info!("Not at version 0, nothing to do. This migrarion probably should be removed");


Spelling error in 'migrarion'; please update it to 'migration'.

Suggested change

info!("Not at version 0, nothing to do. This migrarion probably should be removed");

info!("Not at version 0, nothing to do. This migration probably should be removed");

dmitrylavrenov · 2025-03-05T15:21:20Z

@MOZGIII the idea is still relevant ? Or we would like to leave migration logic at our pallets as it is

In case, it's relevant, I'll rebase over latest master

MOZGIII · 2025-04-20T22:16:22Z

This should be a part of one of the substrate upgrades, but it is not explicitly marked as such, and doesn't contain the necessary context for when a similar change in the thought was introduced to substrate.

the idea is still relevant ? Or we would like to leave migration logic at our pallets as it is

This is, really, very simple. We want to follow the lead of the upstream here. So all that's necessary to decide is the context from above. Please add it to the PR description, and it will be quite trivial to make a decision.

Most likely we'll go with the upgrade. If it's not yet what the substrate was doing - we'll postpone it. If this is something that had to be included with our current substrate version - let's apply it.

MOZGIII

The changes here are technically fine - but there is still a question of whether we want to switch to this approach or not.

See #1315 (comment)

dmitrylavrenov force-pushed the rework-migration-logic branch 2 times, most recently from 3323773 to 2cb6dc3 Compare November 1, 2024 12:42

dmitrylavrenov changed the title ~~Rework migration logic~~ Rework migration and on runtime upgrade logic execution Nov 1, 2024

dmitrylavrenov force-pushed the rework-migration-logic branch 9 times, most recently from 10f7009 to ab80d9c Compare November 7, 2024 10:25

dmitrylavrenov changed the base branch from master to imporve-runtime-logs November 7, 2024 10:25

dmitrylavrenov force-pushed the rework-migration-logic branch 2 times, most recently from ab80d9c to 48a39c7 Compare November 7, 2024 15:09

dmitrylavrenov changed the base branch from imporve-runtime-logs to master November 7, 2024 15:09

dmitrylavrenov force-pushed the rework-migration-logic branch 6 times, most recently from 0af952a to b78da82 Compare November 8, 2024 12:41

dmitrylavrenov marked this pull request as ready for review November 8, 2024 12:44

dmitrylavrenov requested review from MOZGIII and quasiyoke November 8, 2024 12:44

MOZGIII reviewed Nov 8, 2024

View reviewed changes

crates/humanode-runtime/src/init_storage_version.rs Show resolved Hide resolved

MOZGIII reviewed Nov 8, 2024

View reviewed changes

crates/pallet-balanced-currency-swap-bridges-initializer/src/tests.rs Outdated Show resolved Hide resolved

MOZGIII requested changes Nov 8, 2024

View reviewed changes

dmitrylavrenov requested a review from MOZGIII November 8, 2024 14:40

dmitrylavrenov force-pushed the rework-migration-logic branch from 05b4304 to c5d8330 Compare November 20, 2024 13:54

dmitrylavrenov marked this pull request as ready for review November 21, 2024 12:53

dmitrylavrenov added 15 commits November 25, 2024 14:21

Rework migration for pallet-erc20-support

af1b501

Rework migration for pallet-humanode-session

556f2ef

Rework on runtime upgrade logic for pallet-dummy-precompiles-code

39214bc

Fix error type in try-runtime code

08c62fb

Fix test for pallet-dummpy-precompiles-code

4b263f1

Add migrations to humanode-runtime

9f8ccc9

Remove not expected check

3e71309

Rework on runtime upgrade logic for pallet-balanced-currency-swap-bri…

4e8aa38

…dges-initializer

Rename migration name

4a49be2

Use assert_eq instead of ensure

fadf850

Improve info message

9ad5aba

Use log instead of sp_tracing

dba2f4c

Improve log messages

758cddd

Properly run on_runtime_upgrade in tests

758a776

Don't run useless migrations at runtime

9ff37a1

dmitrylavrenov force-pushed the rework-migration-logic branch from e14e9b8 to 9ff37a1 Compare November 25, 2024 11:21

MOZGIII reviewed Nov 29, 2024

View reviewed changes

crates/pallet-dummy-precompiles-code/src/mock/v1.rs Show resolved Hide resolved

MOZGIII requested a review from Copilot March 4, 2025 21:45

Copilot AI reviewed Mar 4, 2025

View reviewed changes

dmitrylavrenov marked this pull request as draft March 11, 2025 07:52

This was referenced Mar 24, 2025

Remove already applied pallet_multisig::migrations::v1::MigrateToV1 at humanode-runtime migrations scope #1499

Merged

Finalize migrations pattern management #1500

Open

MOZGIII approved these changes Apr 20, 2025

View reviewed changes

	return Err("precompiles not created properly: {:not_created_precompiles}");
	return Err(format!("precompiles not created properly: {:?}", not_created_precompiles).as_str());

	info!("Not at version 0, nothing to do. This migrarion probably should be removed");
	info!("Not at version 0, nothing to do. This migration probably should be removed");

Conversation

dmitrylavrenov commented Nov 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MOZGIII left a comment

Choose a reason for hiding this comment

Uh oh!

dmitrylavrenov commented Nov 8, 2024

Uh oh!

MOZGIII commented Nov 21, 2024

Uh oh!

dmitrylavrenov commented Nov 21, 2024

Uh oh!

MOZGIII Nov 29, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

PR Overview

Reviewed Changes

Uh oh!

Copilot AI Mar 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 4, 2025

Choose a reason for hiding this comment

Uh oh!

dmitrylavrenov commented Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MOZGIII commented Apr 20, 2025

Uh oh!

MOZGIII left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

dmitrylavrenov commented Nov 1, 2024 •

edited

Loading

dmitrylavrenov commented Mar 5, 2025 •

edited

Loading

MOZGIII left a comment •

edited

Loading